Add Process.on_terminate
#13694
Add Process.on_terminate
#13694
Conversation
|
@HertzDevil @straight-shoota what do you think? |
|
This impl suffers from the same problem that is described for Signal#trap here: #13803 |
|
This PR is independent from #13803, although the relative positions between the previous handler parameter and the reason parameter could introduce a breaking change if not done carefully. |
|
Any thoughts on this one? Would be nice to simplify this situation: {% if flag?(:win32) %}
Process.on_interrupt { shutdown_gracefully }
{% else %}
# Detect ctr-c
Signal::INT.trap { shutdown_gracefully }
# Docker containers use the term signal
Signal::TERM.trap { shutdown_gracefully }
{% end %} |
|
It would be preferrable to discuss the problem in an independent issue and then think about potential solutions. Being presented with an implemented solution without a detailled and open analysis of the issue it tries to solve may hinder the problem solving process. It's great that you already did the impementation work. And making concrete suggestions is certainly helpful. But a pull request with a concrete implementation isn't a great place to discuss solution strategies openly. The previous comment shows a great example of why this is useful. I've created issue #13983 so we can talk independent of this particular implementation. Until that's come to a conclusion, this PR should be a draft. |
| @@ -58,10 +58,35 @@ struct Crystal::System::Process | |||
| raise RuntimeError.from_errno("kill") if ret < 0 | |||
| end | |||
|
|
|||
| @[Deprecated("Use `#on_terminate` instead")] | |||
There was a problem hiding this comment.
thought: IMO it would not necessary to deprecate this method. It's just the backend of Process.on_interrupt and should never be called directly. It doesn't hurt to have the deprecation, though.
There was a problem hiding this comment.
yeah I put it there so it would be simpler to find and remove at a later date. Let me know if you want me to remove
There was a problem hiding this comment.
No, I think it's fine. Thanks!
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
|
@HertzDevil what do you think? |
|
@HertzDevil any further changes you can think of? |
straight-shoota
changed the title
Process.on_terminate
|
This pull request has been mentioned on Crystal Forum. There might be relevant details there: https://forum.crystal-lang.org/t/upcoming-1-12-0-release/6714/1 |
This fixes some inconsistences between the platforms
Also improves usefulness
Replaces #13654 as I screwed up my rebase
Resolves #13983